Capsule foremanctl certificate bundle deploy#21325
Conversation
Reviewer's GuideAdds a new parameterized test and shared fixture to validate Sequence diagram for foremanctl certificate-bundle fixture behaviorsequenceDiagram
participant Test as pytest
participant Sat as sat
participant Capsule as rhel_contenthost
Test->>Sat: install_satellite_foremanctl(parameters)
alt certificate_source == custom_server
Test->>Sat: custom_cert_generate(sat.hostname)
Test->>Sat: install_satellite_foremanctl(parameters=[--certificate-source=custom_server])
Test->>Sat: execute("foremanctl certificate-bundle --certificate-server-certificate /root/hostname/hostname.crt --certificate-server-key /root/hostname/hostname.key", Capsule, timeout="30m")
Sat-->>Test: deploy (status)
Test->>Sat: execute("tar tzf /var/lib/foremanctl/certs/bundles/rhel_contenthost.tar.gz")
Sat-->>Test: result (status)
else certificate_source == default
Test->>Sat: install_satellite_foremanctl()
Test->>Sat: execute("foremanctl certificate-bundle rhel_contenthost", timeout="30m")
Sat-->>Test: deploy (status)
Test->>Sat: execute("tar tzf /var/lib/foremanctl/certs/bundles/rhel_contenthost.tar.gz")
Sat-->>Test: result (status)
end
Test-->>Test: assert deploy.status == 0
Test-->>Test: assert result.status == 0
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
867559d to
36990ca
Compare
There was a problem hiding this comment.
Hey - I've found 2 security issues, 3 other issues, and left some high level feedback:
Security issues:
- Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
- Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option. (link)
General comments:
- In
install_capsule_foremanctl,cer_sourceandhostnameare required for theforemanctl certificate-bundlecommand but default toNoneand are never passed from the fixture, which will cause a failure when" ".join(cer_source)is called; consider making them required parameters and validating them before use. - The new upstream-specific branch in
setup_capsule_reposis not used byinstall_capsule_foremanctl, which currently callssetup_satellite_repos; verify whether Capsule installs should instead callsetup_capsule_reposso the upstream Capsule repo setup logic is actually exercised.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `install_capsule_foremanctl`, `cer_source` and `hostname` are required for the `foremanctl certificate-bundle` command but default to `None` and are never passed from the fixture, which will cause a failure when `" ".join(cer_source)` is called; consider making them required parameters and validating them before use.
- The new upstream-specific branch in `setup_capsule_repos` is not used by `install_capsule_foremanctl`, which currently calls `setup_satellite_repos`; verify whether Capsule installs should instead call `setup_capsule_repos` so the upstream Capsule repo setup logic is actually exercised.
## Individual Comments
### Comment 1
<location path="robottelo/hosts.py" line_range="2041-2050" />
<code_context>
+ cer_source=None,
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle `cer_source` more defensively before joining it into the command.
`cer_source` can be `None`, but later you call `" ".join(cer_source)`, which will fail if it’s `None` or not an iterable of strings. Either make `cer_source` required (and document it as a list/tuple of strings), or add validation and raise a clear error if it’s missing/invalid.
</issue_to_address>
### Comment 2
<location path="robottelo/hosts.py" line_range="2042-2051" />
<code_context>
+ hostname=None,
</code_context>
<issue_to_address>
**issue (bug_risk):** Avoid passing a `None` hostname through to the `foremanctl certificate-bundle` command.
If `hostname` remains `None`, the command will literally include `None`, which is likely invalid and hard to diagnose. Please either require `hostname` or validate it before constructing the command and fail fast with a clear error if it’s missing or empty.
</issue_to_address>
### Comment 3
<location path="tests/foreman/installer/test_install_foremanctl.py" line_range="211" />
<code_context>
+ capsule = module_capsule_foremanctl_custom_certs
+ common_sat_install_assertions(capsule)
+ # check the services are up and healthy
+ result = capsule.execute('hammer ping')
+ assert result.status == 0
+ assert result.stdout.count('Status:') == result.stdout.count(' ok')
</code_context>
<issue_to_address>
**issue (testing):** The test does not assert that the served certificate CN matches the Capsule hostname as described in the PR.
This test only checks `hammer ping` and an HTTPS 200 with the custom CA. Please add an assertion that inspects the server certificate CN (for example via `openssl s_client` or `curl --resolve` with verbose output) and verifies it equals `capsule.hostname`, so the test confirms the expected certificate is actually presented for that hostname.
</issue_to_address>
### Comment 4
<location path="robottelo/hosts.py" line_range="2079-2082" />
<code_context>
self.execute(
f'foremanctl certificate-bundle --certificate-source {" ".join(cer_source)} {hostname} ',
timeout='30m',
).status
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
*Source: opengrep*
</issue_to_address>
### Comment 5
<location path="robottelo/hosts.py" line_range="2096-2099" />
<code_context>
self.execute(
f'foremanctl deploy {" ".join(default_parameters)}',
timeout='30m',
).status
</code_context>
<issue_to_address>
**security (python.sqlalchemy.security.sqlalchemy-execute-raw-query):** Avoiding SQL string concatenation: untrusted input concatenated with raw SQL query can result in SQL Injection. In order to execute raw query safely, prepared statement should be used. SQLAlchemy provides TextualSQL to easily used prepared statement with named parameters. For complex SQL composition, use SQL Expression Language or Schema Definition Language. In most cases, SQLAlchemy ORM will be a better option.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
08c11c5 to
d87d52d
Compare
664d2f7 to
046d3f1
Compare
|
PRT Result |
046d3f1 to
71c5be0
Compare
|
This pull request has not been updated in the past 45 days. |
|
This pull request is now being closed after stale warnings. |
71c5be0 to
00fd5d6
Compare
5f90cfb to
5cba060
Compare
|
PRT Result |
5cba060 to
b6239b5
Compare
|
PRT Result |
b6239b5 to
02a85f2
Compare
|
PRT Result |
02a85f2 to
4743166
Compare
|
PRT Result |
bcc88fe to
67bd144
Compare
|
PRT Result |
67bd144 to
b93df3c
Compare
|
|
PRT Result |
|
|
||
| # Phase 1: initial Satellite validity | ||
| assert_cert_validity_days(sat, sat_certs, DEFAULT_CERT_DAYS) | ||
| assert_cert_validity_days(sat, capsule_certs, DEFAULT_CERT_DAYS) |
There was a problem hiding this comment.
This still does not validate the certificates inside the bundle.
There was a problem hiding this comment.
It is being verified here. Is not sufficient?
There was a problem hiding this comment.
that only verifies the bundle has a file called {hostname}.crt, not the content of the file
There was a problem hiding this comment.
We already test the presence of the files in https://github.com/theforeman/foremanctl/blob/master/tests/certificate_bundle_test.py, no need to repeat that (IMHO)
The "did the bundle change (or not change) properly depending on user input" is a valuable test.
There was a problem hiding this comment.
Ah, this is already verified in the unit tests, so we can skip it.
b93df3c to
4882ed7
Compare
4882ed7 to
39933c1
Compare
39933c1 to
be5bcd2
Compare
|
Why is it deselecting all our tests?! |
Validate that the
foremanctl certificate-bundlecommand successfully generates a valid certificate bundle tarball for a given Capsule hostname. The validation ensures that the generated tarball contains all required certificate files, including CA, server, and client certificates, and verifies the integrity and consistency of duplicated certificate pairs by confirming they are byte-identical.Summary by Sourcery
Add automated validation of foremanctl certificate bundle generation and renewal for Capsules using both default and custom server certificates.
Tests: